-
Notifications
You must be signed in to change notification settings - Fork 213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor handling of baseCore, and take enrichment into account (zorkow/speech-rule-engine#462) #617
Conversation
…hment into account, and so that more computation is done in the common wrappers. Rename script to scriptChild for sonsistency with other similar values.
PS, this supersedes #605. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please clarify my two questions? Then I think I can finish it off quickly.
It's certainly a much cleaner effort than what I attempted.
return (corebox.ic ? 1.05 * corebox.ic + .05 : 0); | ||
public getBaseFence(fence: W, id: string): W { | ||
if (!fence || !fence.node.attributes || !id) { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that not simply return fence
? This is related to my question above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually in this view it is below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. The three cases here are cases where the fence has not been found, but only the first one has fence
being null
, so if you returned fence
in the other two cases, that would indicate that you found a semantic fence when you really didn't. You only want to return the fence
when it exists, is a node with attributes (e.g., not a text node), and the data-semantic-id
attribute matches the (non-empty) id
that you are looking for. That is what line 322 below does.
Note that this was your code from your original PR (with the addition of the check for the attributes).
* @return {W} The wrapper for the base core mi or mo (or whatever) | ||
*/ | ||
public getBaseCore(): W { | ||
let core = this.getSemanticBase() || this.childNodes[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This confuses me: If the semantic base exists, would that not be a wrapper and therefore I would have take the first child?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wrapper is the output-jax-specific shell around the actual (internal) MathML node, node an enclosing MathML node. If the fence is an mo
(an MmlMo
node), for example, then getSemanticBase()
will return the CHTMLmo
object that is the wrapper for that MmlMo
node. You don't need to take its children, because it is already the wrapper that you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks for the explanation. It makes sense. I just thought we could avoid the return null
part, but it does not look like it.
No problem. Thanks for the original PR that prompted this updated version! |
This PR refactors the handling of the base core element for munderover and msubsup elements so that super- and subscripts can be placed properly after SRE enrichment. It also lays the groundwork for better TeX emulation for accents and over- and underlines (which will be in separate PRs).
The base core element, along its italic correction and relative scaling factor, are now found during the
constructor()
rather than calling them (multiple times) when needed. The base core is used for vertical positioning of super- and subscripts, but the actual base for width and bounding box computations. The handling of the base core BBox and script BBoxes have been moved to the common wrappers, where possible.For consistency, the
script
property has been change toscriptChild
, to be comparable to the other child pointers.In
common/Wrappers/scriptbase.ts
, because thecoreIC()
andcoreScale()
methods have been removed and new function added in that location, the diff gets a little mixed up about the changes, so it's a bit complicated to look at. You might view the actual file for that bit in order to read it easier.Resolves issue Speech-Rule-Engine/speech-rule-engine#462.